-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/#550 Confirm Modal 구현 #553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다
`; | ||
const ButtonFlex = styled(Flex)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`; | |
const ButtonFlex = styled(Flex)` | |
`; | |
const ButtonFlex = styled(Flex)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개행 부분이랑 style lint 수정완료 했어요
`; | ||
const Title = styled.header` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`; | |
const Title = styled.header` | |
`; | |
const Title = styled.header` |
`; | ||
const Content = styled.div``; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`; | |
const Content = styled.div``; | |
`; | |
const Content = styled.div``; |
`; | ||
const ConfirmButton = styled.button` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`; | |
const ConfirmButton = styled.button` | |
`; | |
const ConfirmButton = styled.button` |
const onKeyDown = useCallback((event: KeyboardEvent) => { | ||
const { key } = event; | ||
if (key === 'Enter') { | ||
event.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 왜 필요한가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
포커스가 trigger button에 있어서 엔터 누르면 트리거가 실행됩니당. focus를 강제 이동시키는 방법도 있는데, 텍스트(타이틀)에 포커스를 두게 하는 게 좋은 것인지는 찾아봐야할 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분을 enter이벤트를 막기 보다는, title로 포커스를 강제 이동하는 방식으로 변경했습니다! 물론 title이 상호작용하진 않지만, 트리거 버튼에서 포커스를 해제시키는 게 좋을 것 같아서요
const onKeyDown = useCallback((event: KeyboardEvent) => { | ||
const { key } = event; | ||
if (key === 'Enter') { | ||
event.preventDefault(); | ||
resolveConfirmation(false); | ||
closeModal(); | ||
} | ||
|
||
if (key === 'Escape') { | ||
resolveConfirmation(false); | ||
closeModal(); | ||
} | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 스토리북에서 테스트 해봤는데 저만 동작 안하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOpen
의존성에 추가해주었어요! 원래 ConfirmModal
에 넣어주었다가 Provider
로 옮기는 리팩터링 과정에서 실수했네요!
resolveConfirmation(false); | ||
closeModal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 onDeny와 구분한 이유가 있나요?
const ConfirmProvider = ({ children }: { children: ReactNode }) => { | ||
const [isOpen, setIsOpen] = useState(false); | ||
const resolverRef = useRef<{ | ||
resolve: (value: boolean | PromiseLike<boolean>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 PromiseLike 타입은 왜 있는건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
숙원 사업 하나씩 깨부셔가고 있네요 ㅎㅎ
고생이 많습니다 우코~! 코멘트 확인해주시고 다시 요청주세요~ ㅎㅎ
const confirm = (contents: ModalContents) => { | ||
openModal(); | ||
setModalContents(contents); | ||
|
||
const promise = new Promise<boolean>((resolve) => { | ||
resolverRef.current = { resolve }; | ||
}); | ||
|
||
return promise; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 이 함수를 컨펌 모달 사용처에서 사용하면 되는 것 같은데요~!
내부적으로 promise가 필요한 이유는 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirm을 하기 전 까지는 다른 작업을 막을 수 있습니다.
const clickHiByeBtn = async () => { | ||
const isConfirmed = await confirm({ | ||
title: '하이바이 모달', | ||
content: ( | ||
<> | ||
<p>도밥은 정말 도밥입니까?</p> | ||
<p>코난은 정말 코난입니까?</p> | ||
</> | ||
), | ||
denial: '바이', | ||
confirmation: '하이', | ||
}); | ||
|
||
if (isConfirmed) { | ||
alert('confirmed'); | ||
return; | ||
} | ||
|
||
alert('denied'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 저는 처음에 모달을 보고 사용처에서 onDeny, onConfirmed 등의 props로 실행할 함수를 전달해서 사용할 수 있을 거라 예상했어요.
예상과는 다르게, 스토리북코드에서 처럼 confirm함수의 결과물로 나온 boolean 값으로 명령형 로직을 작성해야 하더라고요.
onDeny, onConfirmed는 따로 조작해야 하도록 하신 이유가 있을까요?
우코의 컴포넌트 작성의도가 궁금해요~ 설명을 해주시면 좋겠습니당~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도밥 말처럼 onDeny, onConfirmed로 함수를 인자로 넣을 수 있게 만들어도 됩니당!
하지만 모든 것을 props로 받는것 보다
핸들러 로직에서 confirm이 되었을 때 ~~를 하겠다고 명령형으로 적는 게 코드 읽는 데 더 편하지 않을까요?
https://developer.mozilla.org/en-US/docs/Web/API/Window/confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isConfirmed = await confirm({
title: '오쁜클로즈 모달',
content: (
<>
<p>코난은 정말 코난입니까?</p>
<p>도밥은 정말 도밥입니까?</p>
</>
),
});
if (isConfirmed) {
alert('confirmed');
return;
}
alert('denied');
저는 명령형 로직을 작성하지 않아도 된다는게 react의 장점이라고 생각해요.
const doA = () => {}
const doB = () => {}
return <ConfirmModal onDeny={doA} onConfirm={doB}></ConfirmModal>
처음에는 우코가 작성한 인터페이스가 낯설고 익숙하지 않아서
그 의도를 이해하지 못했는데요~! 지금도 우코의 생각을 모두 이해한것 같지는 않지만, 컨펌 모달을 전역으로 사용할 수 있다는 규제 완화? 와 함께 합리적인 인터페이스를 구현하셨다고 생각이 들어요.
사용해보고 나중에 또 이야기해 보고싶네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const doA = () => {}
const doB = () => {}
// 많은 코드...
return <ConfirmModal onDeny={doA} onConfirm={doB}></ConfirmModal>
이 사이에 많은 코드들이 추가된다면 어떻게 될까요?같은 역할을 하는 코드가 흩어져서 위아래로 번갈아가면서 읽어야할 것 같아요!
제가 만든 훅은 JSX(UI)와 로직을 한 데 뭉쳐두는 것이 목적이었습니다. UI 부분은 선언적으로 처리하고, 수락/거부했을 때 로직은 명령형으로 작성했습니다.
저는 선언적이라고 반드시 좋고, 명령형이라고 해서 반드시 나쁘다고 생각하지 않아요.
만약 책의 표지에서 제목(선언적 표현)만으로 독자에게 어떤 책인지 표현할 수 없다면, 부제나 요약설명(절차적 표현)을 밖으로 드러내는 것과 같다고 봅니다. 책 겉표지에서 제대로 이해가 안되면 독자는 내부를 읽어볼 수밖에 없으니까요.
interface ModalContents { | ||
title: string; | ||
content: ReactNode; | ||
denial?: string; | ||
confirmation?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 denial, confirmation 메세지가 옵셔널이네요~
혹시 실제 Modal에 기본값과 옵셔널 처리를 하지않고 여기서 한 이유가 있을까요~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 요거 위쪽의 ConfirmModalProps랑 비슷한 타입이라서 재사용을 해볼 수도 있을것 같기도 하고요~ 의도를 몰라서 우선 남겨봅니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
denial, confirmation 메세지가 옵셔널이네요~
이 부분은 버튼 이름입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 위쪽의 ConfirmModalProps랑 비슷한 타입이라서 재사용을 해볼 수도 있을것 같기도 하고요~ 의도를 몰라서 우선 남겨봅니다~
제가 조심스럽게 타입을 사용하는 경향이 있어요! 우선 ConfirmModalProps
는 이제 modal 을 사용하는 곳에서 사용할 interface이고, ModalContents
는 내부에서 관리하고 있는 상태에요! 지금은 비슷해 보이면서도 성격이 다른 것 같아서 분리해 놓았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 버튼 이름입니다!
아 네엡 저도 버튼의 메세지를 말한거였습니당
실제 Modal에 기본값과 옵셔널 처리를 하지않고 여기서 한 이유가 있을까요~?
이게 메인 질문이었어요!
top: 0; | ||
right: 0; | ||
bottom: 0; | ||
left: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inset
으로 1줄로 줄여볼까용~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋아요!
margin: 0 auto; | ||
padding: 24px; | ||
|
||
color: #ffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 theme .color.white 레츠고우~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 함께 이야기 나누면 좋을 것 같아용. 항상 color 값 줄때마다 ${({theme}) => theme.color.white}
를 적는 게 불편한데 도밥은 어떠신가요? 저는 차라리 객체로 관리하는 게 낫지 않을까 생각도 듭니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공감합니다. 테마를 동적으로 바꾸는 일도 없어서 더욱더 그렇네용
다만 일단은 컨벤션에 맞게 작성해두고, 한번에 바꾸는게 좋을 것 같아요
저도 현 구조에서 theme의 장점이 있을지 한번 더 생각해볼게요!
(한번 생각해봤는데 우선은 없네요 ㅋㅋㅋ...)
confirmation?: string; | ||
} | ||
|
||
const ConfirmProvider = ({ children }: { children: ReactNode }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 저는 children이 옵셔널한게 추후에 해당 provider의 쓰임이 변경될때 조금 더 유연하게 대처할 수 있다고 생각해요
우코는 어떻게 생각하나요~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provider에 children을 optional하게 두면, children을 넣지 않을 수도 있다는 뜻인가요? 그러면 context api를 어느 컴포넌트에서도 사용하지 못하게 될 것 같은데, "조금 더 유연하게 대처"할 수 있다는 도밥의 의견을 더 자세하게 설명해주시면 고려해볼게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러면 context api를 어느 컴포넌트에서도 사용하지 못하게 될 것 같은데,
이 부분을 조금 다르게 말해서, chidren을 사용하지 않게 되는 경우에도 열려있게 하면 어떨까 했어요. 컴포넌트 구현이 수정될 수도 있으니까요~
예를들면 저번에 children: ReactNode[ ] 로 지정되었던 Context처럼 당시에는 '절대 다르게 쓰일일이 없다~' 라고 생각하고 type을 좁여 놓았다가,
나중에 ReactNode로 사용하고 싶을때 컴포넌트를 수정해야 했던것 처럼요.
하지만 우코의 말대로 옵셔널하게 열지 않고 좁혀놓아도 좋다고 생각합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 그때는 타입을 잘못 이해하고 있어서 사용했던 거였어요!
type ReactNode =
| ReactElement
| string
| number
| Iterable<ReactNode>
| ReactPortal
| boolean
| null
| undefined
ReactNode 타입에 기본적으로 Iterable<ReactNode>
이 있더라고요! 그래서 그때는 실수였습니다.
그거와는 별개로 현재상황에서는 ReactNode를 Optional이면 안된다고 생각해요. Provider을 사용한다는 것이 하위 React Element에서 Context를 주입할 수 있다는 점이니까요. 예상되는 문제점, 혹은 예상되지 않더라도 혹여나 문제가 될거라 짐작되는 부분이 있으면 언제든 말씀해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우코~~~~ㅎ 다시 코멘트 달아봤어용~
확인해주시고 답변만 주세용ㅎㅎ
고생하셨습니다~😀
interface ModalContents { | ||
title: string; | ||
content: ReactNode; | ||
denial?: string; | ||
confirmation?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 버튼 이름입니다!
아 네엡 저도 버튼의 메세지를 말한거였습니당
실제 Modal에 기본값과 옵셔널 처리를 하지않고 여기서 한 이유가 있을까요~?
이게 메인 질문이었어요!
dom && dom.focus(); | ||
}; | ||
|
||
return createPortal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
포탈이 모달에 1번, provider에 1번 총 2번 적용되어있어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정 완료했습니다!
); | ||
}; | ||
|
||
export default ConfirmProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 문든 궁금한 부분인데, Modal이라는 네이밍은 의도적으로 쓰지 않으신건가용?
파일명과는 다르게 ConfirmProvider, ConfirmContext 라고 되어있어서요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아뇨아뇨! 추가했습니다!
import { useContext } from 'react'; | ||
import { ConfirmContext } from '../ConfirmModalProvider'; | ||
|
||
export const useConfirm = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 이부분도 Modal이라는 네이밍은 일부러 제거하신건가요~?
개인적으로는 useConfirm이라고만 하면 Context를 사용하는 훅인지 모를수도 있을 것 같아요!
Modal이라는 네이밍이 붙으면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름에 Context 추가했습니다!! 불필요하게 너무 길어지는 것 같아서 Modal은 넣지 않고 대신 반환 함수의 이름을 조금 더 자세하게 변경했어요!
margin: 0 auto; | ||
padding: 24px; | ||
|
||
color: #ffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공감합니다. 테마를 동적으로 바꾸는 일도 없어서 더욱더 그렇네용
다만 일단은 컨벤션에 맞게 작성해두고, 한번에 바꾸는게 좋을 것 같아요
저도 현 구조에서 theme의 장점이 있을지 한번 더 생각해볼게요!
(한번 생각해봤는데 우선은 없네요 ㅋㅋㅋ...)
const clickHiByeBtn = async () => { | ||
const isConfirmed = await confirm({ | ||
title: '하이바이 모달', | ||
content: ( | ||
<> | ||
<p>도밥은 정말 도밥입니까?</p> | ||
<p>코난은 정말 코난입니까?</p> | ||
</> | ||
), | ||
denial: '바이', | ||
confirmation: '하이', | ||
}); | ||
|
||
if (isConfirmed) { | ||
alert('confirmed'); | ||
return; | ||
} | ||
|
||
alert('denied'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isConfirmed = await confirm({
title: '오쁜클로즈 모달',
content: (
<>
<p>코난은 정말 코난입니까?</p>
<p>도밥은 정말 도밥입니까?</p>
</>
),
});
if (isConfirmed) {
alert('confirmed');
return;
}
alert('denied');
저는 명령형 로직을 작성하지 않아도 된다는게 react의 장점이라고 생각해요.
const doA = () => {}
const doB = () => {}
return <ConfirmModal onDeny={doA} onConfirm={doB}></ConfirmModal>
처음에는 우코가 작성한 인터페이스가 낯설고 익숙하지 않아서
그 의도를 이해하지 못했는데요~! 지금도 우코의 생각을 모두 이해한것 같지는 않지만, 컨펌 모달을 전역으로 사용할 수 있다는 규제 완화? 와 함께 합리적인 인터페이스를 구현하셨다고 생각이 들어요.
사용해보고 나중에 또 이야기해 보고싶네요 ㅎㅎ
confirmation?: string; | ||
} | ||
|
||
const ConfirmProvider = ({ children }: { children: ReactNode }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러면 context api를 어느 컴포넌트에서도 사용하지 못하게 될 것 같은데,
이 부분을 조금 다르게 말해서, chidren을 사용하지 않게 되는 경우에도 열려있게 하면 어떨까 했어요. 컴포넌트 구현이 수정될 수도 있으니까요~
예를들면 저번에 children: ReactNode[ ] 로 지정되었던 Context처럼 당시에는 '절대 다르게 쓰일일이 없다~' 라고 생각하고 type을 좁여 놓았다가,
나중에 ReactNode로 사용하고 싶을때 컴포넌트를 수정해야 했던것 처럼요.
하지만 우코의 말대로 옵셔널하게 열지 않고 좁혀놓아도 좋다고 생각합니다~
- ConfirmProvider > ConfirmModalProvider - useConfirm > useConfirmContext - confirm > confirmPopup
📝작업 내용
전역에서 사용할 Confirm Modal을 구현하였습니다.
useConfirm
을 사용하여 confirm 함수를 사용하시면 됩니다.💬리뷰 참고사항
useConfirm
사용법title
,content
,denial
,confirmation
을 객체로 인자를 받습니다.content
타입은 ReactNode이며 나머지는 string만 받습니다.denial
,confirmation
은 버튼의 이름을 뜻합니다. 기본값은 닫기와 확인입니다.Promise<boolean>
을 반환합니다.기존 코드에서 confirm 성격의 모달을 사용하는 곳에 아직 대체하지 않았습니다. 해당 PR이 approve 받으면 변경 적용하는 작업을 별도의 PR로 하겠습니다. 이유는 리뷰 중에 해당 구현이 변경될 수 있기 때문입니다.
#️⃣연관된 이슈
close #550